fix(#398): audit Turnstile failure handling, add regression tests - #533
Open
Vyacheslav-Tomashevskiy wants to merge 1 commit into
Open
Conversation
…ression tests
Audited src/lib/turnstile.ts and its two call sites (login, register):
- Missing token, Cloudflare rejection, network error, and a malformed
(non-JSON) response from Cloudflare all already return a structured
{success:false, reason} instead of throwing.
- Both auth/login and auth/register already turn that into a 400
(errorResponse(`Verification failed: ${reason}`, 400)) instead of a
generic 500.
So the behavior the issue asks to confirm was already correct - the
actual gap was that none of it had a single test, so nothing would
catch a regression if it broke. Added:
- src/lib/__tests__/turnstile.test.ts: unit tests for every branch -
no secret configured (dev skip), missing/empty token, Cloudflare
success, Cloudflare rejection with error-codes, rejection without
error-codes (fallback reason), network failure, and a non-JSON
response body.
- src/app/api/auth/login/__tests__/route.test.ts: route-level
regression test proving a failed Turnstile check surfaces as 400
with the exact user-facing message, not 500 - and that the route
short-circuits before ever touching the database (no supabase mock
needed in this file; if the route tried to query it after a failed
check, the test would throw instead of silently passing).
9 new tests, all passing. Verified live via npm run dev: POST
/api/auth/login with an empty turnstileToken returns 400
{"success":false,"error":"Please complete the verification
challenge"}, not 500.
Like Northgate-Systems#529/Northgate-Systems#530/Northgate-Systems#531/Northgate-Systems#532 before it, this branch is cut from a main
that still has the pre-existing missing isValidStellarPublicKey
import bug (from Northgate-Systems#527, fix pending in Northgate-Systems#529), which breaks module-load
for anything importing validations.ts - including the login route,
since it uses loginSchema. Confirmed the 9 new tests genuinely pass by
temporarily restoring the one missing import line locally, running
the full suite (73/75, the other 2 failures are the same
already-reported validations.test.ts fixture issue, unrelated to this
change), then reverting before this commit. npm run build also
verified clean with that same temporary local fix in place.
eslint and tsc --noEmit clean on every file this PR touches.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Someone is attempting to deploy a commit to the codex723's projects Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #398.
What I found
Audited
src/lib/turnstile.tsand its two call sites (api/auth/login,api/auth/register). The failure handling was already correct:{success: false, reason}instead of throwing.400(errorResponse(`Verification failed: ${reason}`, 400)), not a generic500.The actual gap was that none of it had a single test - so nothing would catch it if this ever regressed.
What I did
src/lib/__tests__/turnstile.test.ts— unit tests for every branch: no secret configured (dev skip), missing/empty token, Cloudflare success, Cloudflare rejection witherror-codes, rejection withouterror-codes(fallback reason), network failure, and a non-JSON response body.src/app/api/auth/login/__tests__/route.test.ts— route-level regression test proving a failed Turnstile check surfaces as400with the exact user-facing message, not500- and that the route short-circuits before touching the database (no supabase mock needed in this file; if the route tried to query it after a failed check, the test would throw instead of silently passing).9 new tests, all passing.
Verified live
🔴 Heads up for review, same as #529/#530/#531/#532: a clean
maincheckout currently has a pre-existing bug from #527 (missingisValidStellarPublicKeyimport invalidations.ts, fix pending in #529) that breaks module-load for anything importingvalidations.ts- including the login route, since it usesloginSchema. I confirmed the 9 new tests genuinely pass by temporarily restoring the one missing import line locally, running the full suite (73/75 - the other 2 failures are the same already-reportedvalidations.test.tsfixture issue, unrelated to this change), then reverting before committing.npm run buildalso verified clean with that same temporary local fix in place.eslint/tsc --noEmitclean on every file this PR touches.